refactor: replace ConnectionService with ServiceUtility #1534
Conversation
…sterAwareWriterFailoverHandler
aaron-congo
force-pushed
the
service-container-utility
branch
from
3e3ca46 to
c465a00
Compare
aaron-congo
changed the title
| } | ||
|
|
||
| protected FullServicesContainer getNewServicesContainer() throws SQLException { | ||
| return ServiceUtility.getInstance().createServiceContainer( |
There was a problem hiding this comment.
Just a suggestion. I've seen such piece of code at least twice. I think it makes sense to create a new method:
ServiceUtility.createNewFromSource(FullServicesContainer source, PluginService pluginService, Properties props)
There was a problem hiding this comment.
Is it the same pluginService that is sitting in serviceContainer? Can we use
this.servicesContainer.getPluginService() instead?
There was a problem hiding this comment.
Good idea, thanks!
There was a problem hiding this comment.
I added the requested method, I did however keep the "getNewServicesContainer" method because it allows me to mock the service container creation in our unit tests
wrapper/src/main/java/software/amazon/jdbc/util/ServiceUtility.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/util/ServiceUtility.java
Outdated
Show resolved
Hide resolved
| * {@link software.amazon.jdbc.util.ServiceUtility#createServiceContainer} followed by | ||
| * {@link PluginService#forceConnect} instead. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
We're going to release 3.0 with some breaking changes. Do we want to add one more breaking change and completely remove this interface and implementation class?
There was a problem hiding this comment.
Sounds like we will keep these changes as part of a minor version bump instead of a major one, but I agree we should remove this when we bump to 3.0
2 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.